Feature implementation from commits 15fd731..a0815c8 #2
Conversation
* Add createdAt filter to realtime subscribing with tags * Filter realtime colums and expose ability to skip some columns * Add sharding support for electric * Use unkey cache for the created at filter caching * Remove 2 unused indexes on TaskRun * Run list now filters by a single runtime environment * Remove project ID indexes * Use clickhouse in task list aggregation queries instead of pg (keep pg for self-hosters) * WIP clickhouse powered runs list stuff * Improve the query to get the latest tasks for the task list presenter * Update the usage task list to use clickhouse * Implement next runs list powered by clickhouse * Add new index for TaskRun for the runs list, by environment ID * Add runTags gin index * Handle possibly malicious inputs * Ignore claude settings * Better handling not finding an environment on the schedule page * Use ms since epoch in test, not seconds * Remove unused function * Fix test * Use an env var for the realtime maximum createdAt filter duration (defaults to 1 day) * Fixed the query builder to correct the group by / order by order * Make sure runs.list still works * Create small-birds-arrive.md
…commits (triggerdotdev#2163) * v4: fix stuck batch issue when processing happens before transaction commits * Fix flaky test
### PR: Optimize **TaskRun** indexes for hot-path queries **What changed** | Object | Type | Purpose | | ------------------------------- | --------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------- | | `taskrun_runtime_id_desc_idx` | **BTREE** `(runtimeEnvironmentId, id DESC) INCLUDE (createdAt)` | Eliminates explicit sort for the “latest task runs” query (`ORDER BY id DESC`) while remaining index-only. | | `taskrun_runtime_createdat_idx` | **BTREE** `(runtimeEnvironmentId, createdAt DESC) INCLUDE (id)` | Accelerates the filter-only path that scans by `createdAt >= …` without any ordering requirement. | | `taskrun_createdat_brin` | **BRIN** on `createdAt` (`pages_per_range = 128`) | Lets the planner skip whole blocks older than the time window for both queries at < 100 MB cost. | | *(cleanup)* | **DROP** `TaskRun_runtimeEnvironmentId_createdAt_id_idx` | Retires the 3-column index once the new ones are built. | **Key details** * All indexes created **CONCURRENTLY** to avoid write blocking. * `fillfactor = 90` on b-trees for balanced space vs. future growth. * Net disk usage drops **≈ 15–20 GB** while each query now gets a purpose-built access path. **Why** * Remove planner Sort nodes for the top-N “latest runs” view. * Speed up environment-filtered range scans. * Shrink index bloat and improve cache efficiency.
* Added docs * Improved docs and added video * Added upgrade badge
* Install the kapa sdk * WIP using the SDK for the Kapa Ask AI widget * Removes old kapa from root * Now rendering everything inside the dialog component * Fixes min-height of dialog content * Remove kapa from root * prevents kapa using reCaptcha * Adds more functionailty with temporary UI placement for now * Reset conversation button * Adds a new sparkle list icon * Adds some example questions as a blank state * Animate in the example questions * use “marked” package to render markdown * Improve some animations * Submit a question from the URL param * adds custom scroll bar styling * fixes modal to correct height after re-opening it * Add button to stop generating answer mid-stream * Adds buttons states to show submitting, generating, submittable * Adds a helpfull sentence in the blank state * Show a message if the chat returns an error * Adds reset chat and feedback buttons to the bottom of an answer * Makes sure you can give feedback in the different states of chat * Adds a suble background to the dialog * Fix a button inside button error * Improve the shortcut esc key on dialog and sheet component * Fix classname error * organize imports * Use our custom focus-visible * Move the Tooltip for the button into the AskAI component * Improved error message * Organize imports * Animated the modal gradient * Small layout improvements * Adds most asked questions from Kapa * border glow tweak * AskAI component is now a hook that can take a question * remove kapa script * Add a delay before the modal opens when usign the URL params * Remove old component * Update to the latest Kapa version * Rephrased error message * Use correct types for conversation * Fixed types for addFeedback * Adds DOMPurify package * removed unused const * Removed unnecessary platform specification * Reset the timeout when the ai panel pops up Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix for coderabbit bad commit * Clean up imports --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Fix realtime re-subscribing stale data issue Fixes an issue with realtime when re-subscribing to a run, that would temporarily display stale data and the changes. Now when re-subscribing to a run only the latest changes will be vended * removed logs
* Move all the logic into a single component, preparing for client only * Use ClientOnly * Fix for search param not working
* Runs filter by org id and add created at to ordering * CopyableText can accept an alternative value for copying * The runs table now shows the ID instead of run number * Paginating back/forwards fix * The task stats need org id and project id too
![codeowlai[bot]](https://avatars.githubusercontent.com/in/1259765?s=60&v=4){kind=small}
| logger.debug("Calling RunListPresenter", { options }); | ||
|
|
||
| const results = await presenter.call(options); | ||
| const results = await presenter.call(environmentId, options); |
There was a problem hiding this comment.
🐛 Correctness Issue
Missing required parameter in presenter.call().
The call to presenter.call() is missing the required environmentId parameter, which would cause a runtime exception.
Current Code (Diff):
- const results = await presenter.call(options);
+ const results = await presenter.call(environmentId, options);📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const results = await presenter.call(environmentId, options); | |
| const results = await presenter.call(environmentId, options); |
| cursor, | ||
| pageSize = DEFAULT_PAGE_SIZE, | ||
| }: RunListOptions) { | ||
| public async call( |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking API Change: Method Signature Modification.
The RunListPresenter.call() method signature has been changed to require environmentId as a separate parameter instead of part of the options object, which will break all existing callers.
Proposed Code:
public async call(
environmentId: string,
{
userId,
projectId,
tasks,
versions,
statuses,
tags,
scheduleId,
period,
bulkId,
isTest,
rootOnly,
batchId,
runIds,
from,
to,
direction = "forward",
cursor,
pageSize = DEFAULT_PAGE_SIZE,
}: RunListOptions
) {
🔄 Dependencies Affected
/app/repositories/org/repo/src/main/typescript/callers.ts
Function: Any function calling RunListPresenter.call()
Issue: All callers of this method must be updated to pass environmentId as a separate parameter rather than as part of the options object
Suggestion: Update all call sites to use the new signature: presenter.call(environmentId, { ...otherOptions })
Current Code (Diff):
- await presenter.call({ environmentId: 'env-123', userId: 'user-456', ... });
+ await presenter.call('env-123', { userId: 'user-456', ... });
| const runPresenter = new RunListPresenter(this.#prismaClient); | ||
|
|
||
| const { runs } = await runPresenter.call({ | ||
| const { runs } = await runPresenter.call(environmentId, { |
There was a problem hiding this comment.
🐛 Correctness Issue
Missing required parameter in method call.
The runPresenter.call() method is missing the required environmentId parameter, which would cause a runtime exception when executed.
Current Code (Diff):
- const { runs } = await runPresenter.call({
+ const { runs } = await runPresenter.call(environmentId, {📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const { runs } = await runPresenter.call(environmentId, { | |
| const { runs } = await runPresenter.call(environmentId, { |
|
|
||
| export const clickhouseClient = singleton("clickhouseClient", initializeClickhouseClient); | ||
|
|
||
| function initializeClickhouseClient() { |
There was a problem hiding this comment.
🐛 Correctness Issue
Missing return type annotation.
The function returns undefined or a ClickHouse instance without proper type annotation, which could lead to runtime type errors.
Current Code (Diff):
- function initializeClickhouseClient() {
+ function initializeClickhouseClient(): ClickHouse | undefined {📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| function initializeClickhouseClient() { | |
| function initializeClickhouseClient(): ClickHouse | undefined { |
| enabled: env.CLICKHOUSE_KEEP_ALIVE_ENABLED === "1", | ||
| idleSocketTtl: env.CLICKHOUSE_KEEP_ALIVE_IDLE_SOCKET_TTL_MS, |
There was a problem hiding this comment.
🐛 Correctness Issue
Missing type validation for environment variables.
Environment variables used in keepAlive configuration lack type checking, which could cause runtime errors if values are incorrect.
Current Code (Diff):
enabled: env.CLICKHOUSE_KEEP_ALIVE_ENABLED === "1",
- idleSocketTtl: env.CLICKHOUSE_KEEP_ALIVE_IDLE_SOCKET_TTL_MS,
+ idleSocketTtl: Number(env.CLICKHOUSE_KEEP_ALIVE_IDLE_SOCKET_TTL_MS) || undefined,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| enabled: env.CLICKHOUSE_KEEP_ALIVE_ENABLED === "1", | |
| idleSocketTtl: env.CLICKHOUSE_KEEP_ALIVE_IDLE_SOCKET_TTL_MS, | |
| enabled: env.CLICKHOUSE_KEEP_ALIVE_ENABLED === "1", | |
| idleSocketTtl: Number(env.CLICKHOUSE_KEEP_ALIVE_IDLE_SOCKET_TTL_MS) || undefined, |
| compression: { | ||
| request: true, | ||
| }, | ||
| maxOpenConnections: env.CLICKHOUSE_MAX_OPEN_CONNECTIONS, |
There was a problem hiding this comment.
🐛 Correctness Issue
Missing validation for maxOpenConnections.
The maxOpenConnections parameter is used without validation, which could cause performance issues or crashes if the value is invalid.
Current Code (Diff):
- maxOpenConnections: env.CLICKHOUSE_MAX_OPEN_CONNECTIONS,
+ maxOpenConnections: Number(env.CLICKHOUSE_MAX_OPEN_CONNECTIONS) || undefined,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| maxOpenConnections: env.CLICKHOUSE_MAX_OPEN_CONNECTIONS, | |
| maxOpenConnections: Number(env.CLICKHOUSE_MAX_OPEN_CONNECTIONS) || undefined, |
| electricUrl.searchParams.set("handle", electricUrl.searchParams.get("shape_id") ?? ""); | ||
| } | ||
|
|
||
| const skipColumnsRaw = $url.searchParams.get("skipColumns"); |
There was a problem hiding this comment.
🐛 Correctness Issue
Undefined variable reference causing runtime error.
The code references '$url' which appears to be undefined, likely meant to use 'url' instead, which would cause a runtime exception.
Current Code (Diff):
- const skipColumnsRaw = $url.searchParams.get("skipColumns");
+ const skipColumnsRaw = url.searchParams.get("skipColumns");📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const skipColumnsRaw = $url.searchParams.get("skipColumns"); | |
| const skipColumnsRaw = url.searchParams.get("skipColumns"); |
| // Runs after each test in the current context | ||
| afterEach(async () => { | ||
| // Clean up resources here | ||
| await runsReplicationService.stop(); | ||
| }); |
There was a problem hiding this comment.
🐛 Correctness Issue
Improper test cleanup registration.
Using afterEach inside a utility function registers cleanup for all tests that use this function, causing unpredictable test behavior and potential resource leaks.
Current Code (Diff):
- // Runs after each test in the current context
- afterEach(async () => {
- // Clean up resources here
- await runsReplicationService.stop();
- });
-
- return {
- clickhouse,
- };📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| // Runs after each test in the current context | |
| afterEach(async () => { | |
| // Clean up resources here | |
| await runsReplicationService.stop(); | |
| }); | |
| // Return cleanup function instead of registering it directly | |
| return { | |
| clickhouse, | |
| cleanup: async () => { | |
| await runsReplicationService.stop(); | |
| } | |
| }; |
| clickhouseUrl: string; | ||
| redisOptions: RedisOptions; | ||
| }) { | ||
| await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`); |
There was a problem hiding this comment.
🔒 Security Issue
Unsafe database schema modification.
Direct schema modification with $executeRawUnsafe without environment checks could alter production database tables if the utility is accidentally used in a production context.
Current Code (Diff):
- await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`);
+ // Check environment before modifying schema
+ if (process.env.NODE_ENV === 'production') {
+ throw new Error('Cannot modify database schema in production environment');
+ }
+ await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`);📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`); | |
| // Check environment before modifying schema | |
| if (process.env.NODE_ENV === 'production') { | |
| throw new Error('Cannot modify database schema in production environment'); | |
| } | |
| await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`); |
| } | ||
|
|
||
| return runShapeStream<TRunTypes>( | ||
| `${this.baseUrl}/realtime/v1/runs/${runId}${queryParams ? `?${queryParams}` : ""}`, |
There was a problem hiding this comment.
🐛 Correctness Issue
URLSearchParams object always evaluates as truthy.
The code checks if queryParams is truthy, but as a URLSearchParams object it's always truthy even when empty, which could lead to malformed URLs with trailing question marks.
Current Code (Diff):
- `${this.baseUrl}/realtime/v1/runs/${runId}${queryParams ? `?${queryParams}` : ""}`,
+ `${this.baseUrl}/realtime/v1/runs/${runId}${queryParams.toString() ? `?${queryParams}` : ""}`,📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| `${this.baseUrl}/realtime/v1/runs/${runId}${queryParams ? `?${queryParams}` : ""}`, | |
| `${this.baseUrl}/realtime/v1/runs/${runId}${queryParams.toString() ? `?${queryParams}` : ""}`, |
|
|
||
| try { | ||
| const updatedKeys = new Set<string>(); | ||
| let isUpToDate = false; |
There was a problem hiding this comment.
🐛 Correctness Issue
Reference Error: Removed variable still in use.
The code removes the 'updatedKeys' declaration but the variable is still referenced elsewhere in the code, which will cause runtime errors.
Current Code (Diff):
- let isUpToDate = false;
+ const updatedKeys = new Set<string>();
+ let isUpToDate = false;📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| let isUpToDate = false; | |
| const updatedKeys = new Set<string>(); | |
| let isUpToDate = false; |
🔄 Dependencies Affected
packages
Function: ReadableShapeStream.transform
Issue: The transform method still uses updatedKeys.add(key) in multiple places
Suggestion: Keep the updatedKeys declaration to prevent reference errors
| // Now enqueue only one updated row per key, after all messages have been processed. | ||
| if (!this.#isStreamClosed) { | ||
| // If the stream is not up to date, we don't want to enqueue any rows. | ||
| if (!this.#isStreamClosed && isUpToDate) { |
There was a problem hiding this comment.
🐛 Correctness Issue
Potential data processing blockage.
The new condition requires an 'up-to-date' message before enqueueing any rows, which could prevent data processing if such messages aren't guaranteed in all scenarios.
Current Code (Diff):
- if (!this.#isStreamClosed && isUpToDate) {
+ if (!this.#isStreamClosed && (isUpToDate || this.#currentState.size > 0)) {📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| if (!this.#isStreamClosed && isUpToDate) { | |
| if (!this.#isStreamClosed && (isUpToDate || this.#currentState.size > 0)) { |
|
|
||
| async function processRealtimeRun<TTask extends AnyTask = AnyTask>( | ||
| runId: string, | ||
| filters: { skipColumns?: RealtimeRunSkipColumns }, |
There was a problem hiding this comment.
🐛 Correctness Issue
Breaking API change without updating caller.
Adding a required parameter to processRealtimeRun will break existing calls in useRealtimeRun function that don't pass this parameter.
🔄 Dependencies Affected
packages/useRealtime.ts
Function: useRealtimeRun
Issue: Function calls processRealtimeRun without the new required filters parameter
Suggestion: Update the function call to include the new filters parameter
Current Code (Diff):
await processRealtimeRun(
runId,
+ { skipColumns: options?.skipColumns },
apiClient,
mutateRun,
setError,
abortControllerRef,
typeof options?.stopOnCompletion === "boolean" ? options.stopOnCompletion : true
);
|
|
||
| const timeoutSignal = AbortSignal.timeout(10000); | ||
|
|
||
| const $signal = AbortSignal.any([signal, timeoutSignal]); |
There was a problem hiding this comment.
🐛 Correctness Issue
AbortSignal.any() compatibility issue.
AbortSignal.any() is only available in Node.js 17.3.0+ and will cause runtime errors in older versions.
Current Code (Diff):
- const $signal = AbortSignal.any([signal, timeoutSignal]);
+ // Create a composite abort controller for compatibility with older Node.js versions
+ const compositeController = new AbortController();
+ signal.addEventListener('abort', () => compositeController.abort());
+ timeoutSignal.addEventListener('abort', () => compositeController.abort());
+ const $signal = compositeController.signal;📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const $signal = AbortSignal.any([signal, timeoutSignal]); | |
| // Create a composite abort controller for compatibility with older Node.js versions | |
| const compositeController = new AbortController(); | |
| signal.addEventListener('abort', () => compositeController.abort()); | |
| timeoutSignal.addEventListener('abort', () => compositeController.abort()); | |
| const $signal = compositeController.signal; |
| } | ||
| ); | ||
|
|
||
| const timeoutSignal = AbortSignal.timeout(10000); |
There was a problem hiding this comment.
🐛 Correctness Issue
AbortSignal.timeout() compatibility issue.
AbortSignal.timeout() is only available in Node.js 15.0.0+ and will cause runtime errors in older versions.
Current Code (Diff):
- const timeoutSignal = AbortSignal.timeout(10000);
+ const timeoutController = new AbortController();
+ setTimeout(10000).then(() => timeoutController.abort());
+ const timeoutSignal = timeoutController.signal;📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| const timeoutSignal = AbortSignal.timeout(10000); | |
| const timeoutController = new AbortController(); | |
| setTimeout(10000).then(() => timeoutController.abort()); | |
| const timeoutSignal = timeoutController.signal; |
| cron: { | ||
| pattern: "0 5 * * *", | ||
| timezone: "Asia/Tokyo", | ||
| environments: ["PRODUCTION"], // Only run in production |
There was a problem hiding this comment.
🐛 Correctness Issue
Empty task implementation enabled in production.
The scheduled task is being enabled for production but has an empty implementation, which will cause it to run daily without performing any work.
🔄 Dependencies Affected
v3-catalog/src/trigger/scheduled.ts
Function: secondScheduledTask.run
Issue: The run function is empty but the task is being enabled for production
Suggestion: Implement the run function before enabling the task in production
Current Code (Diff):
- run: async (payload) => {},
+ run: async (payload) => {
+ // Implement the scheduled task logic here
+ },
This PR contains changes from a range of commits from the original repository.
Commit Range:
15fd731..a0815c8Files Changed: 92 (47 programming files)
Programming Ratio: 51.1%
Commits included:
... and 5 more commits